London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python #142
London | 25-SDC-July | Andrei Filippov | Sprint 4 | Implement shell tools in Python #142Droid-An wants to merge 11 commits intoCodeYourFuture:mainfrom
Conversation
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on these implementations - I've left a comment on each file with a pointer to show you where you could improve them.
For your question about formatters what do you mean - formatting your code or formatting strings?
- You can use any code formatter (or none at all), as long as you are consistent with the code you commit
- Python has a few ways to do string formatting, the newest versions of python have something called "f-strings" that are very powerful, but might take a lot of time to learn fully
implement-shell-tools/cat/cat.py
Outdated
| for path in args.path: | ||
| with open(path) as file: | ||
| lines = file.readlines() | ||
| line_num = 1 |
There was a problem hiding this comment.
When printing out the line numbers, compare how the original cat application works to yours. Do you see any discrepancies?
There was a problem hiding this comment.
The only discrepancies I see are in spacing. Is that acceptable, provided the output is correct?
implement-shell-tools/ls/ls.py
Outdated
| path_proceeding(args.path) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
This is a very minor remark, but try not to leave lots of blank lines throughout the file, like here at the end.
implement-shell-tools/wc/wc.py
Outdated
| total.append(output.copy()) | ||
| output.append(path) | ||
| string_list = map(str, output) | ||
| print(" ".join(string_list)) |
There was a problem hiding this comment.
Can you see any problem that might happen with the displayed spacing of the output?
LonMcGregor
left a comment
There was a problem hiding this comment.
- For cat, try running the original cat program with the
-nargument, with multiple files using a wildcard, and then run your own. Can you see any differences? - Can you see any issues with the alignment of columns if you run ẁc`with multiple files using a wildcard?
|
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work, I think you are almost there with this implementation.
| if os.path.isfile(path): | ||
| with open(path) as file: | ||
| lines = file.readlines() | ||
| line_num = 1 |
There was a problem hiding this comment.
If I pass in multiple files the numbering still isn't quite working right here. Think about when you might want to use cat with multiple files, and why you might number the lines. How should the application behave in those cases?
There was a problem hiding this comment.
Sorry, I’m not sure what part isn’t working. When I run the cat command in the console with the -n flag and multiple files, I get this output:
cyf@cyfs-MacBook-Pro cat % cat -n sample-files/*.txt
1 Once upon a time...
1 There was a house made of gingerbread.
1 It looked delicious.
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4
5 There's more to come, though...
When I run the same with my command, I get the same result:
cyf@cyfs-MacBook-Pro cat % python3 cat.py -n sample-files/*.txt
1 Once upon a time...
1 There was a house made of gingerbread.
1 It looked delicious.
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4
5 There's more to come, though...
implement-shell-tools/wc/wc.py
Outdated
| output_for_one_file.append(file_size) | ||
|
|
||
| if len(args.path) > 1: | ||
| numbers.append(output_for_one_file.copy()) |
There was a problem hiding this comment.
Is this variable intended to help store totals? Is "numbers" a good variable name for this?
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. |
|
oh, interesting! cat on linux seems to behave differently to cat on a mac. On linux, when I run cat with multiple files, the numbering is sequential (there's no 1 1 1 as you get, it starts 1 2 3 and the numbering is constant across all files). I think you've met the base requirements of this task, but if you want to stretch yourself, you could try to implement a linux-style cat. Good work on this sprint task! |
Learners, PR Template
Self checklist
Changelist
Implemented shell tools in Python
Questions
what formatter to use for python?